trident-acl-agent: add storm trident-acl-agent E2E test harness, test images, and pipeline - #731

tools/storm/aclagent/proxies/scenario.go:81

• Validate() currently counts assert-failure-reason as a valid mutually-exclusive step kind, but that step kind is not runnable. If removing assert-failure-reason, update the validation error accordingly so invalid scenarios fail fast with a correct message.

		if step.AssertFailureReason != "" {
			kinds++
		}
		if kinds != 1 {
			return fmt.Errorf("scenario step %d must set exactly one of patch/expect/assert-failure-reason", index)

docs/Development/Testing/TridentAclAgent-Tests.md:176

• The "Reboot Choice" section describes shim-based reboot interception, but the harness code treats finalize as a real reboot and waits for SSH to go down/up. The docs should describe the real reboot flow, or the implementation should be updated to actually use a reboot shim.

## Reboot Choice

This scenario uses shim-based reboot interception rather than a full VM
reboot: a `reboot`/`systemctl reboot` shim on `PATH` inside the VM signals the
scenario's controller and exits the agent process instead of actually
rebooting. The scenario then restarts `trident-acl-agent` fresh, exercising
its post-reboot resume logic without tearing down the SSH session or the
in-process fake services. This is less realistic than a full reboot, but it
keeps the test deterministic and fast.

tests/images/trident-vm-testimage/README.md:21

• This section says the ACL-agent image "enables trident-acl-agent.service" and that the scenario writes "localhost" tunnel endpoints, but the base qcow2 config (baseimg-acl-agent.yaml) leaves the service disabled, and the storm harness configures endpoints via --host-endpoint-ip (not localhost reverse tunnels). Updating the README will keep the image docs consistent with the scenario and image configs.

For both, a set of corresponding update images is available. The ACL-agent
variant reuses the servicing-style VM image layout but additionally installs
`trident-acl-agent` and enables `trident-acl-agent.service` so storm ACL-agent
scenarios can drive a real in-guest agent talking to `tridentd`. The image does
not preseed /etc/trident/trident-acl-agent.conf with runner-specific tunnel
ports; the test scenario should SSH in after boot and write the real localhost
proxy endpoints for Nebraska and the Kubernetes API server.

tools/storm/aclagent/README.md:89

• The "Reboot choice" section says the scenario uses a reboot shim to avoid a full reboot, but run-ab-update/run-rollback explicitly wait for SSH to drop and come back, and describe a real systemctl reboot triggered by the agent. The docs should match the harness's real-reboot behavior.

## Reboot choice

This scenario keeps the shim-based reboot interception from the old tester.
That is less realistic than a full VM reboot, but it keeps the test deterministic
and lets the storm runner hold the reverse SSH tunnels and in-process fake services
steady while the agent drives the finalize path.

tools/storm/aclagent/proxies/apiserver.go:285

• handlePatch calls Decoder.DisallowUnknownFields(), but the body is decoded into map[string]any, so unknown fields are never rejected. This can hide mismatches between what the harness expects and what the agent actually patches.

Decode directly into metadataPatch (with DisallowUnknownFields) before applying the patch.

	defer r.Body.Close()
	body := json.NewDecoder(r.Body)
	body.DisallowUnknownFields()
	var raw map[string]any
	if err := body.Decode(&raw); err != nil {
		http.Error(w, fmt.Sprintf("invalid patch body: %v", err), http.StatusBadRequest)
		return
	}

tools/storm/aclagent/README.md:46

• This README describes prepareVmForAclAgent as using localhost:<port> via reverse-SSH-forwarded tunnels and running enable --now, but the harness configures the agent to talk to http://<host-endpoint-ip>:<port> and does an explicit restart (to reconnect between test cases). Keeping the README aligned with the actual harness behavior will reduce confusion when running locally.

`prepareVmForAclAgent` writes `/etc/trident/trident-acl-agent.conf` pointing
at the `localhost:<port>` endpoints storm reverse-SSH-forwards into the VM,
then runs `systemctl enable --now trident-acl-agent.service`. Before that
runs, the service simply isn't started -- no crash-looping, no log noise.

docs/Development/Testing/TridentAclAgent-Tests.md:153

• The "Test Cases" list is out of sync with the current harness: config/service setup happens in run-ab-update/run-rollback (not check-deployment), the scenario includes run-rollback, and the harness performs a real reboot (SSH goes down/up) rather than a shim-based simulated reboot.

1. **deploy-vm** — Copies the base qcow2 image and creates a QEMU VM
2. **check-deployment** — Verifies the VM booted and is accessible via SSH;
   writes `/etc/trident/trident-acl-agent.conf` pointing at the
   `localhost:<port>` endpoints storm reverse-SSH-forwards into the VM, then
   runs `systemctl enable --now trident-acl-agent.service`
3. **run-ab-update** — Starts the fake apiserver and fake Nebraska/Omaha
   endpoints in-process, seeds bootstrap node labels, patches the desired
   update-image label, and waits for `trident-acl-agent` to drive a real
   Trident A/B update to completion (including the shim-based simulated
   reboot)

docs/Development/Testing/TridentAclAgent-Tests.md:159

• The flags table lists --artifacts-dir default as /tmp, but the scenario's TestConfig sets ArtifactsDir default to .. This mismatch can cause local runs to look in the wrong directory by default.

| Flag | Description | Default |
|------|-------------|---------|
| `--artifacts-dir` | Directory containing VM images | `/tmp` |
| `--output-path` | Output directory for logs | `./output` |
| `--platform` | `qemu` or `azure` | `qemu` |
| `--ssh-private-key-path` | Path to SSH private key | `~/.ssh/id_rsa` |
| `--api-server-port` | Port for the fake Kubernetes API server | `18080` |

tools/storm/aclagent/proxies/rp.go:63

• ScenarioStep supports assert-failure-reason (and Scenario.Validate allows it), but RPClient.runStep never handles AssertFailureReason. Any scenario YAML that uses assert-failure-reason will currently fail with "no recognized action" instead of performing the assertion.

Either implement assert-failure-reason handling in the runner, or remove the field (and its validation path) until it’s supported.

func (c *RPClient) runStep(ctx context.Context, index int, step ScenarioStep) (*StepReport, error) {
	switch {
	case step.Patch != nil:
		if err := c.patchNodeRequest(ctx, step.Patch); err != nil {
			return nil, err
		}
		return &StepReport{Index: index, Kind: "patch", Passed: true, Message: "patched fake Node request annotation"}, nil
	case step.Expect != nil:
		return c.expectStatus(ctx, index, step.Expect)
	default:
		return nil, fmt.Errorf("step %d had no recognized action", index)
	}

tools/storm/aclagent/utils/config/config.go:4

• ArtifactsDir default is "." here, but other storm scenarios default it to "/tmp" (e.g. tools/storm/servicing/utils/config/config.go:4 and tools/storm/rollback/utils/config/config.go:4). Keeping this consistent avoids surprising behavior when running different scenarios locally and matches the docs’ stated default.

	ArtifactsDir          string `help:"Directory containing artifacts for the VM" default:"."`

tools/storm/utils/vm/qemu/qemu.go:24

• ImagePattern is compiled as a regex (tools/storm/utils/file/file.go:13) and the default pattern contains an unescaped . before qcow2, so it will also match names like ...testimageXqcow2 and can accidentally match unexpected files (or trigger the "multiple files found" error). Escaping the dot makes the default behave as intended.

	SecureBoot   bool   `help:"Enable secure boot for the VM" default:"false"`
	SerialLog    string `help:"Path to the serial log file" default:"/tmp/trident-vm-verity-test.log"`
	ImagePattern string `help:"Regex pattern used to find the base VM image (.qcow2) in the artifacts directory" default:"^trident-vm-.*-testimage.qcow2$"`

tools/storm/aclagent/README.md:46

• This describes prepareVmForAclAgent as using localhost:<port> via reverse-SSH tunnels and running enable --now, but the implementation writes host-IP endpoints (see tools/storm/aclagent/tests/update.go:262-277) and does systemctl enable + systemctl restart (update.go:360-366). Updating this keeps the doc aligned with the actual harness behavior (and the rationale about tunnels not surviving reboot).

`prepareVmForAclAgent` writes `/etc/trident/trident-acl-agent.conf` pointing
at the `localhost:<port>` endpoints storm reverse-SSH-forwards into the VM,
then runs `systemctl enable --now trident-acl-agent.service`. Before that
runs, the service simply isn't started -- no crash-looping, no log noise.

tools/storm/aclagent/README.md:89

• This section still says the scenario uses shim-based reboot interception and depends on reverse SSH tunnels, but the tests wait for a real reboot by polling SSH reachability (tools/storm/aclagent/tests/update.go:403-429) and explicitly avoid tunnels for the fake endpoints (update.go:124-127). This mismatch will confuse anyone debugging reboot-related failures.

This scenario keeps the shim-based reboot interception from the old tester.
That is less realistic than a full VM reboot, but it keeps the test deterministic
and lets the storm runner hold the reverse SSH tunnels and in-process fake services
steady while the agent drives the finalize path.

docs/Development/Testing/TridentAclAgent-Tests.md:142

• The check-deployment description says the config points at localhost:<port> via reverse-SSH forwarding and that it runs enable --now, but the harness uses a host-reachable IP/port (tests/update.go:262-277) and performs systemctl enable + systemctl restart (update.go:360-366). Aligning this documentation matters because it affects how people reason about connectivity across the real reboot.

1. **deploy-vm** — Copies the base qcow2 image and creates a QEMU VM
2. **check-deployment** — Verifies the VM booted and is accessible via SSH;
   writes `/etc/trident/trident-acl-agent.conf` pointing at the
   `localhost:<port>` endpoints storm reverse-SSH-forwards into the VM, then
   runs `systemctl enable --now trident-acl-agent.service`

tools/storm/aclagent/proxies/rp.go:83

• Issue: decodeStatus() errors are ignored, so a malformed acl.azure.com/update-status annotation will be silently treated as “no status yet” and the step will just time out, hiding the real failure.

Evidence: status, _ := decodeStatus(node) drops the error.

Suggestion: handle the error explicitly and fail the step with the decode error + raw annotation value so the scenario report is self-diagnosing.

		node, err := c.getNode(ctx)
		if err != nil {
			return nil, err
		}
		status, _ := decodeStatus(node)

tools/storm/aclagent/tests/update.go:128

• Issue: The fake apiserver binds to 0.0.0.0, which exposes the unauthenticated test endpoint on all host interfaces. This is avoidable (and can be surprising on developer machines) since the VM only needs it reachable via HostEndpointIP.

Evidence: ListenAndServe(..., "0.0.0.0:<port>").

Suggestion: bind to testConfig.HostEndpointIP (or another explicit listen address) so the server is only exposed where needed.

	// Bind on all interfaces (not 127.0.0.1) so the VM can reach the fake
	// apiserver directly over the libvirt NAT network at testConfig.HostEndpointIP,
	// instead of relying on reverse SSH tunnels. Tunnels don't survive a real
	// VM reboot; a real host IP does.
	if _, err := apiServer.ListenAndServe(ctx, fmt.Sprintf("0.0.0.0:%d", testConfig.APIServerPort)); err != nil {

tools/storm/aclagent/tests/update.go:158

• Issue: The fake image server binds to 0.0.0.0, exposing a local artifact over unauthenticated HTTP on all host interfaces.

Evidence: imageServer.ListenAndServe(..., "0.0.0.0:<port>").

Suggestion: bind to testConfig.HostEndpointIP so it’s only reachable on the interface the VM uses.

			return fmt.Errorf("failed to hash image %s: %w", imagePath, err)
		}
		imageServer := &stormproxies.ImageServer{ImagePath: imagePath}
		if _, err := imageServer.ListenAndServe(ctx, fmt.Sprintf("0.0.0.0:%d", testConfig.ImageServerPort)); err != nil {
			return fmt.Errorf("failed to start fake image server: %w", err)

tools/storm/aclagent/tests/update.go:173

• Issue: The fake Nebraska endpoint binds to 0.0.0.0, exposing an unauthenticated update server mock on all host interfaces.

Evidence: nebraska.ListenAndServe(..., "0.0.0.0:<port>").

Suggestion: bind to testConfig.HostEndpointIP so it’s only reachable on the interface the VM uses.

		SHA384:      nebraskaSHA384,
		PackageName: nebraskaPackageName,
	}}
	if _, err := nebraska.ListenAndServe(ctx, fmt.Sprintf("0.0.0.0:%d", testConfig.NebraskaPort)); err != nil {
		return fmt.Errorf("failed to start fake Nebraska endpoint: %w", err)

tools/storm/aclagent/tests/rollback.go:42

• Issue: The fake apiserver in rollback binds to 0.0.0.0, exposing an unauthenticated test endpoint on all host interfaces.

Evidence: ListenAndServe(..., "0.0.0.0:<port>").

Suggestion: bind to testConfig.HostEndpointIP (or another explicit listen address) to limit exposure to the interface the VM uses.

	nodeStore := stormproxies.NewNodeStore(stormproxies.NewSeedNode(testConfig.NodeName, map[string]string{}))
	apiServer := stormproxies.NewAPIServer(testConfig.NodeName, nodeStore)
	if _, err := apiServer.ListenAndServe(ctx, fmt.Sprintf("0.0.0.0:%d", testConfig.APIServerPort)); err != nil {
		return fmt.Errorf("failed to start fake apiserver: %w", err)
	}

tools/storm/aclagent/trident.go:72

• Issue: On error paths, runTestCase calls tc.FailFromError(err) but then continues and unconditionally logs the test as “passed”. That makes logs misleading and can mask follow-on errors after a known failure.

Evidence: after tc.FailFromError(err) the function falls through to logrus.Infof("test case '%s' passed"...).

Suggestion: return immediately after FailFromError, and only log “passed” when there was no error.

	if testCaseSpecificConfig.OutputPath != "" {
		testCaseSpecificConfig.OutputPath = filepath.Join(testCaseSpecificConfig.OutputPath, tc.Name())
		if err := os.MkdirAll(testCaseSpecificConfig.OutputPath, 0o755); err != nil {
			tc.FailFromError(err)
		}

docs/Development/Testing/TridentAclAgent-Tests.md:142

• Issue: The doc says check-deployment writes config pointing at localhost:<port> via reverse SSH tunnels, but the harness now configures the VM to talk directly to the test runner at HostEndpointIP:<port> (and avoids tunnels specifically because they don’t survive a real reboot).

Evidence: RunABUpdate binds the fake apiserver on 0.0.0.0 and advertises http://<HostEndpointIP>:<port> to the VM.

Suggestion: update this step description to match the actual networking model (host-reachable IP/port, no reverse tunnels).

2. **check-deployment** — Verifies the VM booted and is accessible via SSH;
   writes `/etc/trident/trident-acl-agent.conf` pointing at the
   `localhost:<port>` endpoints storm reverse-SSH-forwards into the VM, then
   runs `systemctl enable --now trident-acl-agent.service`

docs/Development/Testing/TridentAclAgent-Tests.md:157

• Issue: Documented default for --artifacts-dir doesn’t match the scenario’s flag default.

Evidence: docs list /tmp, but TestConfig.ArtifactsDir defaults to ..

Suggestion: update the doc table default (or the flag default) so they stay consistent.

| `--artifacts-dir` | Directory containing VM images | `/tmp` |

tools/storm/aclagent/README.md:46

• Issue: This README says the scenario uses localhost:<port> endpoints via reverse SSH forwarding, but the harness configures the VM to reach the fake services directly on HostEndpointIP:<port> (and intentionally avoids tunnels for reboot survival).

Evidence: prepareVmForAclAgent writes endpoints using HostEndpointIP.

Suggestion: update the README wording to match the actual networking model.

`prepareVmForAclAgent` writes `/etc/trident/trident-acl-agent.conf` pointing
at the `localhost:<port>` endpoints storm reverse-SSH-forwards into the VM,
then runs `systemctl enable --now trident-acl-agent.service`. Before that
runs, the service simply isn't started -- no crash-looping, no log noise.

tests/images/trident-vm-testimage/README.md:25

• Issue: This README says the scenario should write “localhost proxy endpoints”, but the harness configures the VM to talk to the test runner over a host-reachable IP (HostEndpointIP) rather than SSH-tunneled localhost.

Evidence: prepareVmForAclAgent uses HostEndpointIP for the apiserver/Nebraska endpoints.

Suggestion: update this text to describe writing <host-endpoint-ip>:<port> endpoints (reachable from the VM) instead of localhost.

into it there is no test harness left to `systemctl enable --now` it. Neither
image preseeds /etc/trident/trident-acl-agent.conf with runner-specific
tunnel ports; the test scenario should SSH in after boot and write the real
localhost proxy endpoints for Nebraska and the Kubernetes API server.